-
Notifications
You must be signed in to change notification settings - Fork 9
Ace 1568 Proxy support changes #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.DS_Store file is committed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump the version of automate-client-java
to 0.6 in pom.xml
...
this.authentication = new BasicAuthentication(this.username, this.accessKey); | ||
} | ||
|
||
public void setProxy(String proxyHost, int proxyPort, String proxyUsername, String proxyPassword) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so will this method be called by the client using this package? as in... in jenkins plugin..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add javadoc comment for this function since it plays an important role of setting the proxy.
Something like: https://github.com/browserstack/automate-client-java/blob/master/src/main/java/com/browserstack/automate/AutomateClient.java#L41-L46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public void setProxy(String proxyHost, int proxyPort, String proxyUsername, String proxyPassword) { | |
public void setProxy(final String proxyHost, final int proxyPort, final String proxyUsername, final String proxyPassword) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link the Jenkins Plugin PR as well so that it's easier to review in one go...
…ack/automate-client-java into ACE-1568-apache-implementation
Making variables final Co-authored-by: Archish Thakkar <[email protected]>
src/main/java/com/browserstack/client/BrowserStackClientInterface.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
RELATED PR
https://github.com/jenkinsci/browserstack-integration-plugin/pull/28/files
Jenkins plugin support for proxy (Browserstack plugin changes)